Float-bar support. The logic is to use array values [bottomY, topY] i… #5262
Conversation
…nstead of regular vals. SO if you use regular numbers char will be not floated and bottomY will be 0 by default. If you use array for value current update allowing to track this and use bottomY value from array instead of 0.)
|
Thanks for this! This will close numerous issues linked to in #4863 |
|
Hi @simonbrunel, @etimberg! This pull request for adding floating bar support. Can you check pls, so maybe it should be upgraded/changed. This should fix #4863 issue. |
|
I think it's a good start, I need to review it more deeply, but don't have time right now. I agree with the data format We also need to be sure that:
A few early feedback:
|
A few early feedback: instead of duplicating the logic to pick the "right" value in each scale, it may be better to move it directly in getRightValue()? Though, that would need some investigation to be sure it doesn't break anything. I would not introduce the new borderSkipped: 'none' value, but instead support borderSkipped: false|null. I would not call it lowY or highY but low/hight or min/max
|
Hi @simonbrunel @benmccann ! I have made changes according to your suggestions. I have tested all chars, so here the links for you to test it also: vertical char with linear scale horizontal char with linear scale |
|
There are multiple approaches to handle
a. can be achieved without stacking I don't think a. is correct because it's not "stacked" but then I'm not sure between b, c and d. Maybe d. is the best behavior (that's what @etimberg suggested in #4863) because it allows to implement c. as well by mixing |
|
@gwyneblaidd the rebase seems not to have worked. It looks like most of your changes disappeared |
yepp, i had problems with rebasing, everytime smth was in conflict, i had to create new pull request to this one to replace all files so now this pull request has no changes regarding to original repo. But i will return back today all my changes with new commits. |
| @@ -124,11 +124,11 @@ var supportsEventListenerOptions = (function() { | |||
| // https://github.com/chartjs/Chart.js/issues/4287 | |||
| var eventListenerOptions = supportsEventListenerOptions ? {passive: true} : false; | |||
|
|
|||
| function addEventListener(node, type, listener) { | |||
| function addListener(node, type, listener) { | |||
There was a problem hiding this comment.
Not sure why cbb7ff7 is showing up on this PR. Seems the rebase didn't totally work
There was a problem hiding this comment.
when i has all that failing tests i thought that smth really bad went with merge last time. So, i downloaded lates Chart.js files, replaced with it all my local files and commited to my fork. So, in this case my repo should look like the current Chart.js version. So, all that changes came when i replaced files. I have put back my changes again on float-bat support after. Are thise chanages on this file are not correct?
There was a problem hiding this comment.
i have replaced all files from current chart.js version. Is this file is not correct?
There was a problem hiding this comment.
i think because this was commited only 2 days ago thats why i picked it
There was a problem hiding this comment.
The changes shouldn't appear on your PR. The problem is that your PR is comparing to a version of the code from a few days ago probably instead of to the latest master. You should run git rebase master again (and maybe make a backup copy first before doing that)
It's probably really hard to rebase with 82 commits spanning a year on the PR. It's possible you might have an easier time opening a new PR at this point rather than rebasing
There was a problem hiding this comment.
... or you can squash all your 82 commits in a single one, and rebase it on top of master. Then we can keep working in the same PR with all previous comments.
There was a problem hiding this comment.
@gwyneblaidd if you need help squashing / rebasing this PR, please join our Slack #dev channel. But please don't open a new PR with these 82 commits, it will be useless.
There was a problem hiding this comment.
Forget my comment about squashing commits, I didn't realize you already merged many times our master into your branch, which makes the operation quite complicated, so it would be easier to start a new PR from scratch.
There was a problem hiding this comment.
rebased PR - #6056
|
Just for reference, new PR: #6056 |
Float-bar support. The logic is to use array values [bottomY, topY] instead of regular vals. SO if you use regular numbers chart will be not floated and bar start will be 0 by default. If you use array for value current update allowing to track this and use bottomY value from array instead of 0.)
Example:
http://pravopys.net/chartjs/samples/charts/bar/vertical.html